Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Js_of_ocaml TodoMVC Example. #1339

Closed
wants to merge 5 commits into from
Closed

Conversation

slegrand45
Copy link
Contributor

Js_of_ocaml is a compiler of OCaml bytecode to Javascript.

OCaml is a general purpose programming language with an emphasis on expressiveness and safety. Js_of_ocaml brings nearly all the OCaml features to Javascript. And especially its powerful type system.

@arthurvr
Copy link
Member

Personally I think should totally go discuss this at TasteLang but that it isn't really a good fit for TodoMVC. Mainly because this is more about the language than about an MV* framework. What does the rest think?

@passy
Copy link
Member

passy commented Jun 16, 2015

OCaml has a really nice JS story these days and I hear it more and more, especially among people looking for a type-safe way to write web apps, where there aren't very many options out there. The implementation does a pretty nice job of separating the concerns and looks pretty similar to the Elm way, so I'm 👍.


js_of_ocaml +weak.js --opt 3 -o js/todomvc.js todomvc.byte

# Avoid JS linters (hard to apply for auto generated JS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this magic just ignore specific files in the config files.

@arthurvr
Copy link
Member

Any active community member who's able to review the code? I don't think any of us is (or am I wrong?)

@passy
Copy link
Member

passy commented Jun 27, 2015

I'm at a read-only level of OCaml understanding. I'll give it a more
thorough look, but it would indeed be great to have someone with actual
experience in this space to review.

On Fri, Jun 26, 2015, 22:49 Arthur Verschaeve [email protected]
wrote:

Any active community member who's able to review the code? I don't think
any of us is (or am I wrong?)


Reply to this email directly or view it on GitHub
#1339 (comment).

@slegrand45
Copy link
Contributor Author

The code has been reviewed by @Drup who is a member of the jsoo project. The code style is now much better.

@passy
Copy link
Member

passy commented Jun 30, 2015

Oh yes, I prefer the new style a lot over the old let () ... in. Well done! :)

@@ -0,0 +1,2 @@
_build
*.byte
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a newline

@arthurvr
Copy link
Member

Neat work. Thanks.


Various code improvements from [Gabriel Radanne](https://github.com/Drup).

Based on [Elm implementation](https://github.com/evancz) by Evan Czaplicki.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/evancz

make this a link on the name itself

@arthurvr
Copy link
Member

Thanks for your nice work!

<link rel="stylesheet" href="node_modules/todomvc-app-css/index.css">
</head>
<body>
<div id="todomvc"></div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like you are using the 2.0 spec however this root element is an ID, can you switch it to a class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an absolute requirement? The standard way with js_of_ocaml is to select first an element by its id and then use it to plug the application. I could select the <div> element with the getElementsByClassName method but unfortunately this method is for now only available in the development version of js_of_ocaml.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an absolute requirement? I do not think so, however our test suite does a lookup on this element to determine what version of the spec to use when testing, so clearly this does pose some issues.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have an id attribute and a class attribute? ie something like:

<div id="todomvc" class="todoapp"></div>

Otherwise, if the idea is to ban any id attribute, i have no solution for now until the next release of js_of_ocaml (which will include the getElementsByClassName method).

By the way, can you confirm that the needed class name is todoapp? Or is it todomvc? I looked at the file https://github.com/tastejs/todomvc-app-template/blob/master/index.html and i only saw a todoapp class name. Thanks.

@samccone
Copy link
Member

we look for todoapp here is the commit that adds the logic.
45b15b5

For now doing both is an OK solution, thanks for chatting with me, and thanks for the awesome work.

ps I tried compiling this app locally by following your instructions and ran into all kinds of errors when installing dependencies, Do you think you might be able to expand the setup instructions for how to compile this app, thanks!

@slegrand45
Copy link
Contributor Author

I added the "todoapp" class. Thank you for your feedback about your attempt to compile the application. I made a test by starting from a clean environment. And indeed it's not as simple as it seems... So i updated the building instructions. I think it's a lot better now.

@samccone
Copy link
Member

looks like we are in need of a rebase 🌐

@slegrand45
Copy link
Contributor Author

After some fixes, there are still 3 failed checks. They are all from the routing part and they all fail because of "stale element reference: element is not attached to the page document". Not sure if i can apply some workarounds?

@samccone
Copy link
Member

@slegrand45 how recently have you rebased off of master? We Very recently merged a fix #1371

@arthurvr
Copy link
Member

Seems like some unrelated commits got into this PR. Can you rebase and get them out?

@slegrand45 slegrand45 force-pushed the js_of_ocaml branch 2 times, most recently from 0b90397 to df745b3 Compare July 15, 2015 20:37
@slegrand45
Copy link
Contributor Author

It's strange because i already made a rebase... Anyway, i used the following commands:

git checkout js_of_ocaml
git fetch upstream
git rebase upstream/master
git push -f

Strangely enough, i had several conflicts about my own commits during the rebase (?). I used --skip to resolve them. I had also to use "push -f" because git said that my local branch and "origin/js_of_ocaml" have diverged (?). It seems that the PR is now clean again. But i hope i didn't introduce some mess 😟

Besides, the CI log still shows some "stale element reference" (?).

@samccone
Copy link
Member

yep @slegrand45 looks like you uncovered some more flake 👏

@samccone
Copy link
Member

samccone commented Oct 6, 2015

hey @slegrand45 would you mind rebasing this off of master and squashing your commits down?

Js_of_ocaml is a compiler of OCaml bytecode to Javascript.
OCaml is a general purpose programming language with an
emphasis on expressiveness and safety. Js_of_ocaml brings
nearly all the OCaml features to Javascript. And especially
its powerful type system.
@slegrand45
Copy link
Contributor Author

Seems ok!

@samccone
Copy link
Member

samccone commented Oct 9, 2015

@passy I will let you have final 👍 on this

@passy
Copy link
Member

passy commented Oct 9, 2015

@slegrand45 I'm seeing some odd rendering errors among other things. Is the latest version on your branch up to date?

screenshot from 2015-10-09 20-36-54

@slegrand45
Copy link
Contributor Author

@passy Yes it's the latest version. What are the errors you are seeing?

@passy
Copy link
Member

passy commented Oct 9, 2015

Rendering is broken like on the screenshot and I've only tried a few things that all weren't quite right, focus lost after creating an item, no selection when entering edit mode etc. Do you see those things on your end too?

@slegrand45
Copy link
Contributor Author

Ok, i see what you mean. Indeed, i have the same problems. I'm working on it right now to fix it.

@slegrand45
Copy link
Contributor Author

@passy I think it should be better now.

@passy
Copy link
Member

passy commented Oct 10, 2015

@slegrand45 Oh yes, way better! 🌵 I'm checking it now. :)

@passy
Copy link
Member

passy commented Oct 10, 2015

Passing the integration tests with flying colors! Checking with drool now.

screenshot from 2015-10-10 20-40-39

runningcolourdog

@passy
Copy link
Member

passy commented Oct 10, 2015

I took me way longer than it should have to find this GIF, but it was worth it and I'm super excited about landing this. Thanks so much @slegrand45! Also thanks @arthurvr and @samccone for the review! 😻

@passy passy closed this in b25d4e2 Oct 10, 2015
@slegrand45 slegrand45 deleted the js_of_ocaml branch October 10, 2015 20:20
@slegrand45
Copy link
Contributor Author

Thank you very much to all the team! 😃

@LouisAyroles
Copy link

Hello everyone, note sure if it's the right place to ask this :/

Do you have an example with version of dependencies ?
Because I have serious problem to make it work..

When I'm trying with latest opam,eliom config I have problem with js_of_ocaml.deriving.syntax not found.

I've tried many different ways and I'm still stucked..

Thx for yout help. 😄

@slegrand45
Copy link
Contributor Author

slegrand45 commented Jul 20, 2021

Hi,

Indeed, some packages have been renamed since 2015. @LouisAyroles I have sent a PR to fix that: #2155. You can also take a look at the examples at https://github.com/slegrand45/examples_ocsigen. These examples should compile and run without any problem. If not, please send an issue report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants